Skip to content

[BUG]: fix binary search impl of LimitOperator#3489

Merged
codetheweb merged 2 commits intomainfrom
bug-limit-binary-search-off-by-one
Jan 15, 2025
Merged

[BUG]: fix binary search impl of LimitOperator#3489
codetheweb merged 2 commits intomainfrom
bug-limit-binary-search-off-by-one

Conversation

@codetheweb
Copy link
Contributor

@codetheweb codetheweb commented Jan 14, 2025

Description of changes

This has been materializing as a flaky test, but is an actual correctness bug. Reproduced in a Python test and tracked down to this operator.

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Added test was previously failing.

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

n/a

@github-actions
Copy link

github-actions bot commented Jan 14, 2025

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@codetheweb codetheweb force-pushed the bug-limit-binary-search-off-by-one branch from 979bfd5 to ca18e20 Compare January 14, 2025 23:05
Sicheng-Pan
Sicheng-Pan reviewed Jan 14, 2025
View reviewed changes
Sicheng-Pan
Sicheng-Pan approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@Sicheng-Pan Sicheng-Pan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@codetheweb codetheweb force-pushed the bug-limit-binary-search-off-by-one branch from ca18e20 to 69c4d73 Compare January 14, 2025 23:19
@HammadB
Copy link
Collaborator

HammadB commented Jan 15, 2025

Could we hardcode the repro that caused this if it was a prop test?

@codetheweb
Copy link
Contributor Author

codetheweb commented Jan 15, 2025

Could we hardcode the repro that caused this if it was a prop test?

Added

@codetheweb codetheweb enabled auto-merge (squash) January 15, 2025 02:21
@codetheweb codetheweb disabled auto-merge January 15, 2025 17:40
@codetheweb codetheweb merged commit babded1 into main Jan 15, 2025
@codetheweb codetheweb deleted the bug-limit-binary-search-off-by-one branch January 15, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Sicheng-Pan Sicheng-Pan Sicheng-Pan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@codetheweb @HammadB @Sicheng-Pan